fix(engine): Jeweled Amulet notes and reproduces spent mana type (#6504) - #6812

Changes requested — the declared noted-mana class overreaches Ice Cauldron and loses source incarnation.

🔴 Blocker

parser/oracle_effect/imperative.rs:10121-10136 deliberately accepts “note the type and amount of mana,” including Ice Cauldron, but game/effects/mana.rs:544-559 and :852-865 retain only the first noted type and repeat the printed count. The producer parser at parser/oracle_effect/mana.rs:1407-1420 recognizes only “mana of ~'s last noted type.” Ice Cauldron’s current Oracle text is: “Add this artifact's last noted type and amount of mana.” Its X payment must determine the later production amount; this implementation cannot parse or reproduce that full instruction while claiming the class supports it.

Either narrow the change and its claims to singular-type wording, keeping Ice Cauldron strict-unimplemented, or model the noted amount as typed state and add parser plus end-to-end runtime coverage for Ice Cauldron’s activation, stored amount, later production, and spend restriction.

🔴 Blocker

game/effects/note_mana_spent.rs:29-41 reads and writes the latch through raw ability.source_id, while types/ability.rs:22697-22706 provides source_incarnation specifically to prevent rebinding a departed source and game/zones.rs:161-164 keeps an ObjectId as storage identity across zone changes. The new mana_spent_to_activate field at game_object.rs:1134-1149 has no zone-exit cleanup. A bounced or flickered source can therefore receive an old activation’s note on its new incarnation. Capture paid mana and source incarnation in the resolving context, write only if the captured source remains that incarnation, and add a stack-level bounce/flicker regression.

🟡 Required evidence

This engine/parser PR has no current <!-- coverage-parse-diff --> sticky despite a completed Card data job. Publish and reconcile current-head parse-diff evidence. The Rust lint job is also terminal red; resolve its reported gate before re-review.

Recommendation: make the supported class honest or implement the missing type-and-amount and source-incarnation semantics with discriminating end-to-end tests, then regenerate the parser evidence.

Actionable comments posted: 5

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@crates/engine/src/game/ability_scan.rs`:
- Line 4964: Update the ManaProduction::NotedType arm in the ability scan
classification to set sibling: true while preserving its existing count scan.
Add a regression test covering fixed-count NotedType behavior when another
resolution changes the noted mana type, ensuring LoopFirewall detects the
dependency.

In `@crates/engine/src/game/casting.rs`:
- Around line 14725-14739: Move the spent-mana snapshot out of the source-global
latch and bind it to each activated ability/stack entry together with the source
incarnation. In crates/engine/src/game/casting.rs:14725-14739, capture the
payment snapshot during activation; in
crates/engine/src/game/game_object.rs:1134-1149, clear or remove transient
source-level payment state on object changes; and in
crates/engine/src/game/effects/note_mana_spent.rs:29-41, require the matching
incarnation and consume that activation’s snapshot before recording the note.
Add regression coverage for repeated activations across untap and source
re-entry before resolution.

In `@crates/engine/src/game/effects/mana.rs`:
- Around line 544-560: Update the NotedType representation and its handling in
the ManaProduction path to preserve and later reproduce the full mana
composition recorded by the noting activation, including both type and amount
for Ice Cauldron. Replace the first-type repetition and current count resolution
in the NotedType branch with consumption of the durable noted mana value, and
update related support paths around noted_mana_type_for so the reusable
abstraction is not limited to Jeweled Amulet.

In `@crates/engine/tests/integration/issue_6504_jeweled_amulet_noted_mana.rs`:
- Line 77: Update the test around runner.activate and resolve to separate
activation from resolution, then assert that no durable noted mana type is
usable while the activated ability remains unresolved. Ensure the regression
reaches the production mana-payment pipeline and verifies the noted type becomes
available only after NoteManaSpent resolves.
- Around line 127-155: Update jeweled_amulet_tracks_a_different_noted_color to
use the same amulet for two complete note/consume cycles: first note red and
consume the resulting ability, then note green and consume it again through the
existing runner production pipeline. Assert the second cycle produces green and
no red, ensuring the stored noted type is replaced rather than appended and read
stale.

Changes requested — supplemental current-head blockers.

🔴 Blocker

game/ability_scan.rs:4959-4968 treats ManaProduction::NotedType as count-only and never asserts sibling: true, but the producer reads mutable per-source chosen_attributes through effects/mana.rs:856-865. The scanner’s own axis contract at ability_scan.rs:109-116 requires sibling-sensitive state to participate in loop-firewall classification. Add the required sibling axis/coverage so sibling activations cannot be treated as independent when their noted-mana state is shared/mutable.

🟡 Required tests and gate

issue_6504_jeweled_amulet_noted_mana.rs:77 immediately resolves the activation, so it cannot prove the note is absent before resolution; :127-155 uses a new amulet, so it cannot detect overwrite/append behavior on one object. Add reachability tests for both semantics alongside the already requested bounce/flicker case. The current Rust lint failure is the parser-combinator gate rejecting the new string dispatch at parser/oracle_effect/imperative.rs:10128; rewrite it with the repository’s required nom dispatch convention.

The current parse-diff is now present and confirms the earlier semantic concern: it adds NoteManaSpent for Ice Cauldron while adding Mana(NotedType) only for Jeweled Amulet. The earlier type-and-amount and source-incarnation blockers remain unchanged.

Recommendation: address these supplemental authority/test/gate defects together with the existing current-head blockers, then request re-review.

Actionable comments posted: 2

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@crates/engine/src/game/casting.rs`:
- Around line 14725-14744: Bind each activation’s mana-payment snapshot to its
individual pending activation or stack entry, including the source incarnation,
instead of storing it only in GameObject::mana_spent_to_activate. Update the
payment flow around the shown source-mutation logic to attach the snapshot to
the activation record, and update NoteManaSpent resolution to consume that exact
record while rejecting incarnation mismatches; preserve support for both
direct-activation and PendingCast paths without relying on tap-cost behavior.

In `@crates/engine/tests/integration/issue_6504_jeweled_amulet_noted_mana.rs`:
- Around line 211-280: Prove ability 0’s payment was actually recorded before
the bounce by querying P0’s mana pool immediately after ActivateAbility and
asserting the single red mana unit was drained. Add this reach-guard before
capturing incarnation_before or invoking bounce::resolve, using the existing
mana-pool query helper in the test file; preserve the current bounce and final
noted_mana_spent assertions.

Changes requested — current-head paid-mana attribution remains unsafe.

🔴 Blocker

game/casting.rs:14725-14743 writes every activation’s paid mana onto the mutable source-global GameObject::mana_spent_to_activate, explicitly overwriting it on the next activation. effects/note_mana_spent.rs:42-52 later reads that mutable latch and verifies only the object’s current incarnation, not the resolving ability’s own payment. If an unresolved Jeweled Amulet activation is untapped and activated again with a different color, both note effects can read the later payment. Capture the spent mana and source incarnation in the activated stack/ResolvedAbility context, then consume the resolving ability’s snapshot; add a LIFO stacked-activation regression.

🔴 Blocker

game/ability_scan.rs:4959-4968 still treats ManaProduction::NotedType as count-only, even though effects/mana.rs:840-853 reads mutable per-source ChosenAttribute::NotedManaSpent. The scan contract at ability_scan.rs:109-116 classifies that as sibling-mutable state. Assert sibling: true for this producer and add the corresponding loop-firewall regression.

🟡 Required evidence

issue_6504_jeweled_amulet_noted_mana.rs:67-84 immediately resolves the helper activation, so it cannot prove no durable note before resolution/countering; its red and green checks use separate objects (:90-151), so they do not establish replace-versus-append behavior. Add those discriminating cases. The only parse-diff sticky predates this head and still lists Ice Cauldron as supported; regenerate and reconcile current-head evidence for the intentional narrowing.

Recommendation: retain the good narrowing and bounce fix, but make paid-mana provenance per activation, correct the scan axis, add the missing tests, and regenerate parse evidence before re-review.

🔴 Blocker — ability copies retain an activation-only payment snapshot

copy_spell.rs:109-130 clones the entire StackEntryKind, then the activated-ability arm only preserves the original source. That leaves ResolvedAbility::noted_mana_payment (including nested sub_ability/else_ability nodes) intact. note_mana_spent.rs:46-70 consumes that snapshot when the copied ability resolves, so a copied Jeweled Amulet activation can write the original activation's paid mana colors even though the copy did not pay an activation cost.

CR 707.10 says a copied activated ability is not activated. The original's announced decisions and objects used to pay costs are copyable where relevant, but this engine's noted_mana_payment is an activation-payment observation for NoteManaSpent, not a decision or cost object that the copy paid. Preserving it produces a false durable note on resolution.

Please clear noted_mana_payment recursively when copying activated abilities (while retaining the existing CR 707.10b source preservation), and add a runtime regression that copies an activated Jeweled Amulet ability after a colored original payment and proves the copied ability cannot note those original colors. The regression should exercise the copy stack path and resolution rather than only inspecting a constructor.

The existing source-incarnation and per-activation provenance fixes are valuable, but they do not distinguish an original activation from a copy of that activation.

Actionable comments posted: 1

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@crates/engine/tests/integration/issue_6504_jeweled_amulet_noted_mana.rs`:
- Around line 487-595: Add an assertion immediately before the
copy_spell::resolve call that the original stack entry identified by
original_entry_id still carries its noted_mana_payment snapshot. Use the stack
entry’s resolved ability data and assert the snapshot is present, so the later
is_none() check specifically verifies that NoteManaSpent on the copied
activation does not inherit it.

Changes requested — the new effect is still reached through a verbatim, one-card Oracle-text match.

🔴 Blocker

crates/engine/src/parser/oracle_effect/imperative.rs:9765-9782 recognizes the entire normalized sentence "note the type of mana spent to pay this activation cost" in one all_consuming(terminated(tag(...))) branch. The confirmed Jeweled Amulet Oracle text is "Note the type of mana spent to pay this activation cost."; the current parser therefore hard-codes that card sentence rather than parsing the grammatical "note [the] type of mana spent to pay this [activation] cost" structure. This violates the parser's composable-building-block boundary and leaves even straightforward grammatical siblings as unrelated future special cases.

Please move this into a named nom grammar parser that composes the instruction prefix, the noted-mana subject, and the activation-cost referent (with explicit, typed boundaries), then add building-block tests for the accepted structural variants and a negative boundary. Keep Ice Cauldron unsupported unless its distinct amount-and-spend-restriction semantics are modeled end-to-end.

✅ Clean

The current head's per-activation ResolvedAbility::noted_mana_payment snapshot and recursive clearing on copied ability chains address the prior aliasing and copied-ability findings. The copy regression's final original-resolution assertion is also a sufficient positive reach guard for the original snapshot: it would fail if the clone cleanup erased or never preserved it.

Recommendation: request changes for the parser seam, then re-run the current-head parser and runtime evidence.

Actionable comments posted: 1

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@crates/engine/src/parser/oracle_effect/imperative.rs`:
- Around line 13511-13560: The tests only exercise parse_note_mana_spent_clause,
so add end-to-end production parser assertions for both cards. Verify Jeweled
Amulet’s complete wording lowers to Effect::NoteManaSpent, and verify Ice
Cauldron’s type-and-amount wording reaches Effect::Unimplemented; include the
positive reach guard before the negative assertion as required by the parser
test conventions.

🔴 Blocker — current head does not compile

Reviewed commit 56fc5b9e8c18e608b4a12da20966c386df226b66.

Current merge CI is terminal red: Rust lint, Rust tests shard 1, Rust tests shard 2, WASM, Card data, paired-seed AI, and decision-cost perf.

The terminal Rust compiler failures require explicit handling of the new data:

1. ResolvedAbility::noted_mana_payment is omitted from the explicit destructures in crates/engine/src/game/ability_scan.rs:209 and crates/engine/src/game/resolution_prompt.rs:523.
2. Effect::NoteManaSpent is omitted from the exhaustive effect classifications in crates/engine/src/game/ability_scan.rs:408, :5218, and :5933; crates/engine/src/game/resolution_prompt.rs:210; and crates/engine/src/types/ability_visit.rs:276.

Please classify both additions deliberately at every affected seam—whether each is carried, scanned, visited, or intentionally ignored. Do not resolve this with wildcard match arms or destructure ..; these sites represent distinct semantic classifications and must stay explicit. Add or update focused tests for those classifications.

Afterward, provide fresh green CI and a current-head parse-diff. The existing parse-diff predates this head, so it is not evidence for 56fc5b9e8c18e608b4a12da20966c386df226b66.

🔴 Blockers on current head

Reviewed dd95f1c809ed1a9e82663b39c250319e3661ba8b. This is a new review; prior review evidence does not apply to this head.

1. Remove the prohibited dormant-importer mirror. crates/mtgish-import/src/convert/action.rs:304-319 adds ManaProduction::NotedType to rewrite_bound_x_in_mana_production. docs/AI-CONTRIBUTOR.md:91-95 explicitly prohibits modifying mtgish paths or mirroring engine variants there. This hunk must be removed or explicitly reworked under maintainer resolution; “consistency” is not a permitted reason to change a dormant importer.

   I also checked the supplied ancestry claim: 2b204dff59c1e928ab67d4683831deb8b8ff9c15 is an ancestor of 4555043a1d9c5a1f44235fd95b79588d9e12032f, but it does not contain this NotedType arm. git log -S identifies 4555043a… as the introducing commit. That provenance cannot justify retaining the policy-prohibited hunk.

2. Bind source incarnation at payment capture, not later stack construction. casting.rs:15591-15596 records only mana types, while casting_costs.rs:5423-5429 reads the mutable object's incarnation after cost processing. The residual self-return path at casting_costs.rs:3181-3266 can move the source after automatic payment and before that late read, allowing the payment snapshot to bind to the post-zone-change incarnation.

   Capture { types, source_incarnation } atomically at the payment authority, carry that immutable snapshot through every pause/resume and stack continuation, and add a discriminating self-return-cost regression that proves the old payment cannot bind to the new incarnation.

3. The parse-diff sticky is stale. The only <!-- coverage-parse-diff --> evidence says it was generated for 0fc5c36bfd20bf4ce40918d93b8012e31efce1eb, not this head. Current CI is still pending. After it settles, publish and reconcile a fresh parse-diff artifact bound to dd95f1c809ed1a9e82663b39c250319e3661ba8b before re-review.

Maintainer correction

This formal follow-up corrects the prior requested-changes review for unchanged head dd95f1c809ed1a9e82663b39c250319e3661ba8b.

Withdrawn item only: parse_diff_stale_for_current_head is withdrawn. The current parse-diff artifact is bound to this exact head, compares against main 1833435c0d59, and reports exactly one affected card and four signatures for Jeweled Amulet—matching the declared scope.

The requested-changes disposition remains in force for the two substantive blockers: the prohibited dormant mtgish-import mirror and late source-incarnation capture after cost processing. No approval or enqueue is implied by this correction.

Approved for merge queue.

Evidence: final head 2243c0834e has all required checks green; current-head review threads are empty; and the fresh parse-diff artifact reports the intended Jeweled Amulet change only (noted-mana production plus NoteManaSpent). The decision-cost baseline is refreshed from the exact five-sample CI report after the paired-seed gate passed.

🔴 Current-head review — approval overridden

Reviewed 2243c0834e0307f4f9c204c73cbb5f580e106e57. CI and the current parse-diff are clean, but they do not clear these material blockers:

1. Prohibited dormant mirror remains. crates/mtgish-import/src/convert/action.rs:304-319 still adds ManaProduction::NotedType to the mtgish X-rewrite. docs/AI-CONTRIBUTOR.md:91-95 forbids modifying dormant mtgish paths or mirroring new engine variants there. Remove this hunk or obtain explicit maintainer resolution for a genuinely required exception.

2. Source incarnation is still captured after payment and can rebind across a zone change. casting.rs:15591-15596 stores only paid types; casting_costs.rs:5423-5429 later reads obj.incarnation. The self-return residual-cost flow at casting_costs.rs:3181-3266 can move the source between those points, binding the earlier payment to the post-move incarnation.

   Capture { types, source_incarnation } atomically at the payment authority, propagate that snapshot through every pause/resume and stack continuation, and add a discriminating self-return-cost regression proving an old payment cannot bind to a new incarnation.

This changes-requested review supersedes the stale approval. No approval or enqueue pending these fixes.